-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modus dropdown : Updated the Modus dropdown list UI #2749
base: main
Are you sure you want to change the base?
Modus dropdown : Updated the Modus dropdown list UI #2749
Conversation
✅ Deploy Preview for moduswebcomponents ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -123,7 +123,7 @@ export class ModusDropdown { | |||
this.showDropdownListBorder ? 'list-border' : '' | |||
} ${this.animateList ? 'animate-list' : ''} ${this.classByPlacement.get(this.placement)}`; | |||
const left = this.placement === 'right' ? `${this.toggleElement?.offsetWidth}px` : 'unset'; | |||
const width = `${this.toggleElement?.offsetWidth ? this.toggleElement?.offsetWidth : 0}px`; | |||
const width = `${(this.toggleElement?.offsetWidth || 0) < 240 ? 240 : this.toggleElement?.offsetWidth ? this.toggleElement?.offsetWidth : 0}px`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per modus UI, Apply the minimum width 240px.
@@ -58,7 +61,7 @@ export class ModusListItem { | |||
render(): unknown { | |||
const containerClass = `${this.classBySize.get(this.size)} ${this.disabled ? 'disabled' : ''} ${ | |||
this.selected ? 'selected' : '' | |||
} ${this.borderless ? 'borderless' : ''}`; | |||
} ${this.borderless ? 'borderless' : ''} ${this.radiusless ? 'radiusless' : ''}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new prop to make list border less.
@hsoni177 Unfortunately, since we only have the existing guideline that the border is that gray color, we cannot immediately approve this PR. It will need to be discussed if the drop down menu should offer a borderless option. @enowak1031 @prashanth-offcl may need to chime in. Perhaps even @egunther39 |
@cjwinsor Sorry for the misunderstanding here.
So everything is configurable. It does not affect the current behavior and UI except for the box-shadow. |
As per Dafnee: thats ok. Modus Figma file actually displays a dropdown without a border for the autocomplete component (see link below), so I was trying to follow their guidelines. They need to update the Figma and have a consistent look for the dropdown container. The border is fine if that's what Modus wants 👍 https://www.figma.com/design/wyfVJUHWRMkeCfdB38HFEE/Modus---Web?m=auto&node-id=10423-28210&t=JKbWRPUjc6I4ytxe-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hsoni177 Looks like we are trying to solve three issues
- Borderless option for list item component is already implemented so we can skip this.
- The border radius of the list items looks different from Figma. The current implementation is different that what we have in Figma and the style guide. Instead of the proposed property in the PR, may I suggest we apply the border to the modus-list component instead of the list-item component?
- Dropdown menu min width - I'll check with UX and get back on this
Description
I updated the dropdown UI to match our requirements. I also tried to update the dropdown list to match the Modus Figma design, but this change affects the existing modus-list component.
Expected
Modus Has
Updated
References #2611
Type of change
How Has This Been Tested?
Checklist